Repository navigation
Conversation
Retain the SDK version and auth-token lock through exit-owner teardown. Honor Apple system SQLite/zlib even with vcpkg, and let independent embedded copies select a private Objective-C HTTP delegate name without changing the default. Files: CMakeLists.txt; cmake/MatsdkOptions.cmake; docs/embedding-with-cmake.md; lib/CMakeLists.txt; lib/api/AuthTokensController.cpp; lib/pal/PAL.cpp; tests/apple/embedding-options-tests.cmake; tests/apple/embedding-options/CMakeLists.txt; tests/embedding/CMakeLists.txt; tests/embedding/lifetime-tests.cmake; tests/embedding/lifetime-tests.cpp Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 5484aed9-9464-4bc8-9ca7-fd67954034ff
bmehta001
changed the title
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The iOS guard ignores custom delegate names when Apple HTTP is disabled, risking process-global class collisions, and the new embedding test requires CMake 3.19 despite supporting 3.15–3.18.
2 open issues
What changed in this PR
Prepares SDK release 3.10.312.1 and improves shutdown safety and coexistence for independently embedded SDK copies.
Changes:
- Keeps SDK-version and auth-token mutex storage alive for late shutdown workers.
- Uses Apple system SQLite/zlib under vcpkg and adds a configurable private HTTP delegate name.
- Adds lifetime regressions and an 18-case Apple configuration matrix.
| File | Description |
|---|---|
| tests/embedding/lifetime-tests.cpp | Checks state identity and availability during shutdown. |
| tests/embedding/lifetime-tests.cmake | Instruments production sources and registers lifetime tests. |
| tests/embedding/CMakeLists.txt | Integrates lifetime and Apple configuration tests. |
| tests/apple/embedding-options/CMakeLists.txt | Validates dependency targets and delegate definitions. |
| tests/apple/embedding-options-tests.cmake | Runs the 18-case configuration matrix. |
| Package.swift | Updates the binary artifact reference to 3.10.310.1. |
| lib/pal/PAL.cpp | Preserves SDK-version storage through process exit. |
| lib/include/public/Version.hpp | Advances SDK version constants to 3.10.312.1. |
| lib/CMakeLists.txt | Applies the private Apple delegate-name definition. |
| lib/api/AuthTokensController.cpp | Preserves the auth-token mutex through process exit. |
| docs/embedding-with-cmake.md | Documents Apple dependencies and delegate naming. |
| CMakeLists.txt | Resolves Apple system libraries with vcpkg active. |
| cmake/MatsdkOptions.cmake | Adds and validates the delegate-class option. |
Review effort: Balanced
| message(STATUS "Building static SDK library") | ||
| add_library(mat STATIC ${SRCS}) | ||
| endif() | ||
| if(APPLE AND MATSDK_BUILD_APPLE_HTTP) |
| enable_testing() | ||
| add_test(NAME embedding_test COMMAND embedding_test) | ||
| include("${CMAKE_CURRENT_LIST_DIR}/lifetime-tests.cmake") | ||
| add_test(NAME apple_embedding_options |
The placement-new string avoided exit-time destruction but abandoned its heap buffer on unload: two real reload cycles leak 56 bytes under LSan. Return the compile-time literal through the internal const-char accessor instead, preserving late-worker safety without allocation or destruction. Keep the auth mutex and existing Apple embedding fixes unchanged. Files: - lib/pal/PAL.cpp, lib/pal/PAL.hpp: allocation-free internal version accessor. - tests/embedding/lifetime-tests.cpp: string-view value/address expectations. - tests/embedding/lifetime-tests.cmake: entry instrumentation and real DSO test. - tests/embedding/version-reload-fixture.cpp: embed the production SDK archive. - tests/embedding/version-reload-test.cpp: verify two actual unload cycles. - tests/embedding/CMakeLists.txt: opt-in Linux ASan/LSan coverage. - .github/workflows/test-embedding.yml: enforce sanitizer coverage in CI. Validation: Windows VS2026 and Linux GCC13 ASan/LSan embedding suites each pass all six tests. The old placement-new buffer leaks exactly 56 bytes; original destructible statics fail all three lifetime tests on both OSes. Formatting, existing misspell workflow and git diff --check pass. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 5484aed9-9464-4bc8-9ca7-fd67954034ff
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
const char*accessor: no allocation, destructor, or leaked buffer on library unload. Keep the auth mutex in aligned static placement-new storage so late upload workers remain safe; normal SDK worker/resource teardown is unchanged.MATSDK_APPLE_HTTP_DELEGATE_CLASSfor independently embedded Apple SDK copies. The default class name is unchanged; the definition is private to Apple's HTTP implementation and does not rename public wrapper classes.SDK-version leak follow-up (
e630e2c9d11176499856393ffac9179520c80da6)std::stringretained a 28-byte heap buffer per unloaded SDK copy. Two actual unload/reload cycles reproduce a 56-byte LSan leak. A string literal removes the allocation while retaining late-worker safety.PAL.hppis internal, not an installed public API; SDK consumers compile on both Windows and Linux.std::string_viewcontents and.data()identity. Preserve destructor-observing instrumentation for original-SDK negative controls.sdk_version_reload: load a shared fixture containing the real static SDK archive, validate the SDK version, verify actual unload, and repeat. Linux disables GNU-unique pinning in the test archive and uses local symbol binding for the fixture.Follow-up validation
cmake --build out/lifetime-windows --config Release --target embedding_test --parallel 6;ctest --test-dir out/lifetime-windows -C Release --output-on-failure: 6/6 pass./home/bhamehta/build/1ds-upstream-lifetimes, configured with-DMATSDK_EMBEDDING_ADDRESS_SANITIZER=ON: build targetembedding_test; unfiltered CTest: 6/6 pass, including real unload/reload under ASan/LSan.ctest -R '^sdk_version_reload$'fails with 56 bytes leaked in 2 allocations. Restore the literal implementation: passes.ctest -R '^sdk_lifetime_'fails 3/3 with expected destruction diagnostics on Windows and Linux. Restore production fixes: all pass.clang-format --dry-run --Werroron all changed/new C++ tests; changed-linegit clang-format --diff HEADon PAL sources; complete existing cached misspell workflow;git diff --check: all pass.Earlier validation (before the leak follow-up)
git diff --checkpass before push.Scope and limitations